Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: BTC maker/taker fee validation #6814

Merged
merged 1 commit into from Aug 19, 2023
Merged

Fix: BTC maker/taker fee validation #6814

merged 1 commit into from Aug 19, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 16, 2023

Fixes #6813

  • User took an offer at startup, before DAO had finished parsing blockchain.
  • BM addresses were not yet known, so fee validation erroneously flagged the taker fee as unrecognized.

It is another edge case similar to #6810, so protect from false flagging a trade if the DAO is not yet ready.

@ghost ghost changed the title Fix: BTC taker fee validation Fix: BTC maker/taker fee validation Aug 16, 2023
@ghost
Copy link
Author

ghost commented Aug 16, 2023

To clarify: Bisq validates fees paid in Trade Step 2, prior to payment. In this case the app was started up with an existing trade and the validation was allowed to occur too early, before the DAO was ready, causing the erroneous warning.

Copy link
Contributor

@alvasw alvasw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

In this PR, you check with canRequestBeMade() in isServiceSupported whether the request can be made or not. This should cover the change from your previous PR (#6810) too. Therefore, we can safely remove mempoolService.canRequestBeMade(offerPayload) in the TriggerPriceService. Is this correct or am I missing something?

TriggerPriceService:

            OfferPayload offerPayload = offer.getOfferPayload().orElseThrow();
            if (openOffer.getMempoolStatus() < 0 &&
                    mempoolService.canRequestBeMade(offerPayload)) {
                mempoolService.validateOfferMakerTx(offerPayload, (txValidator -> {
                    openOffer.setMempoolStatus(txValidator.isFail() ? 0 : 1);
                }));
            }

Here, mempoolService.validateOfferMakerTx(...), calls validateOfferMakerTx(...), and validateOfferMakerTx() calls isServiceSupported() before creating the mempool request.

MempoolService:

    public void validateOfferMakerTx(TxValidator txValidator, Consumer<TxValidator> resultHandler) {
        if (txValidator.getIsFeeCurrencyBtc() != null && txValidator.getIsFeeCurrencyBtc()) {
            if (!isServiceSupported()) {
                UserThread.runAfter(() -> resultHandler.accept(txValidator.endResult("mempool request not supported, bypassing", true)), 1);
                return;
            }
            MempoolRequest mempoolRequest = new MempoolRequest(preferences, socks5ProxyProvider);
            validateOfferMakerTx(mempoolRequest, txValidator, resultHandler);
        } else {
            // using BSQ for fees
            UserThread.runAfter(() -> resultHandler.accept(txValidator.validateBsqFeeTx(true)), 1);
        }
    }

@ghost
Copy link
Author

ghost commented Aug 16, 2023

canRequestBeMade() ensures that the DAO is ready and we are not flooding too many simultaneous requests.

canRequestBeMade(offer specific) in addition ensures that a new offer is not checked immediately, as the info retrieved from mempool.space may not be available until the Tx has time to propagate.

If TriggerPriceService was simplified to only call validateOfferMakerTx you have the possibility that the offer gets flagged as valid just because the DAO was not yet ready. Or the possibility that a valid offer gets disabled (as missing Tx) before its Tx had chance to propagate.

So I think the validation has to be done after the system becomes ready (i.e. the way it is now).

@alvasw
Copy link
Contributor

alvasw commented Aug 17, 2023

@jmacxx Thank you very much for the explanation!

Copy link
Contributor

@alvasw alvasw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Contributor

@alejandrogarcia83 alejandrogarcia83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@alejandrogarcia83 alejandrogarcia83 merged commit 34d61d1 into bisq-network:master Aug 19, 2023
@alejandrogarcia83 alejandrogarcia83 added this to the v1.9.13 milestone Aug 19, 2023
@ghost ghost mentioned this pull request Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants